Add method to inset polygon vertices #106
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #106 +/- ##
===========================================
- Coverage 79.55% 77.01% -2.54%
===========================================
Files 30 31 +1
Lines 7243 7571 +328
===========================================
+ Hits 5762 5831 +69
- Misses 1481 1740 +259
|
nicklockwood
force-pushed
the
develop
branch
from
797a79e to
c93f7e2
Compare
|
@zilmarinen this method has actually already existed in the develop branch for a while: https://github.com/nicklockwood/Euclid/blob/develop/Sources/Polygon.swift#L342-L360 I didn't ship it yet because of some edge cases (no pun intended) and because I wasn't sure what to do about texture coordinates (should they be kept the same, or also inset proportionally?). I also wanted to implement equivalent inset methods on Mesh and Path at the same time but those raised even more difficult cases. I need to refresh my memory about exactly what the issues were, but if this is something you need I'm happy to prioritize getting it into the next release. Any help you can offer in terms of creating test cases and working around the bugs would be appreciated. |
Yeah sorry I had temporarily removed the WIP stuff from the develop branch in preparation for next release, so it probably wasn't there when you checked for it. |
nicklockwood
force-pushed
the
develop
branch
2 times, most recently
from
59f4ce7 to
8d79842
Compare
|
Apologies for the delayed response. I would like to cover this implementation with some unit tests but I am currently at a loss as to what a decent test for this might look like 🤔 Both convex and non-convex shapes need to be considered here as well as positive and negative values for the inset which starts to add a lot of complexity. And this is only for a single polygon. Considering how to implement insetting on meshes blows my tiny mind! Do you have any considered examples of how this could be tested thoroughly without writing brittle tests that are overly specific to a use case? |
nicklockwood
force-pushed
the
develop
branch
from
8d79842 to
442ed49
Compare
nicklockwood
force-pushed
the
develop
branch
from
442ed49 to
1efcc63
Compare
nicklockwood
force-pushed
the
develop
branch
4 times, most recently
from
ff7f64c to
bdee2ad
Compare
|
@nicklockwood Apologies for letting this PR languish. Insetting is still something that I would like to see implemented in Euclid but as it stands, the implementation I have provided here does not fully satisfy all valid use cases. Your implementation does appear to be more stable - would it be possible to request this is re-added in a future release?
If there are any useful tests or examples I can provide to facilitate this, I am more than happy to assist in moving this forward. |
nicklockwood
force-pushed
the
develop
branch
2 times, most recently
from
91dd394 to
9643e62
Compare
|
Closing this PR as the functionality requested has been added by reintroducing the appropriate |
Description
Implements a new method to inset the vertices of a polygon by a given amount by iterating through edge segment pairs to find a common point of intersection between the two translated planes.
Scope of change
Checklist:
Discussion
Whilst this does appear to yield the correct results, I am unsure if there is a more performant means to achieve the same output.
Searching for the appropriate vertex arbitrarily choosing either
e0.endore1.startviato retain the vertex normal, texture coordinates and color seems like a brittle approach. I can not think of an alternative means to tie the translated vertices back to the original vertex - perhaps you can?
As an aside; I note that a dispacement modifier was suggested here but I am unable to find any implementation of
Mesh.inset()- has this been removed?